feat(ethercat): surface dedup-after-retry ESI uploads in the UI#849
feat(ethercat): surface dedup-after-retry ESI uploads in the UI#849marconetsf wants to merge 4 commits into
Conversation
Shared-surface sync with openplc-web (feat/esi-upload-retry): mirror the EsiPort `dedupAfterRetry` contract field and the ESIUpload consumer that re-lists the repository when an upload is recovered after a transient-failure retry but comes back without an item — so the file surfaces instead of silently vanishing. Adds the byte-identical esi-upload.test.tsx covering the four branches (normal add, dedup-after-retry refetch, refetch-failure fallback, real duplicate). Retry/adapter internals stay web-only. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
WalkthroughThis PR extends the ESI file upload flow to handle a ChangesESI Upload Deduplication After Retry
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/frontend/components/_features/`[workspace]/editor/device/ethercat/components/__tests__/esi-upload.test.tsx:
- Line 1: The test file
src/frontend/components/_features/[workspace]/editor/device/ethercat/components/__tests__/esi-upload.test.tsx
is present in this repo but missing from the openplc-web repository, causing the
surface-sync CI check to fail; either add the identical file to the web repo on
the same target branch or coordinate with the web repo maintainers to merge the
corresponding PR (e.g., the one that should contain esi-upload.test.tsx) before
merging this PR so the byte-identical test is present in both repos and the
compare-surfaces.py check passes.
In
`@src/frontend/components/_features/`[workspace]/editor/device/ethercat/components/esi-upload.tsx:
- Around line 116-127: The UI branch relying on result.dedupAfterRetry is dead
because dedupAfterRetry isn't propagated end-to-end; either implement
propagation or remove the UI branch. To implement propagation: update backend
function parseAndSaveFile in esi-service.ts to set and return
dedupAfterRetry:boolean when appropriate, update the IPC handler
esiParseAndSaveFile in src/main/modules/ipc/renderer.ts to include
dedupAfterRetry in its response, and update the middleware adapter
editor/esi-adapter.ts mapping to pass dedupAfterRetry through when calling
EsiPort.parseAndSaveFile. Alternatively, remove the
needsRepositoryRefresh/dedupAfterRetry handling from the UI component
(esi-upload.tsx) to match current backend behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 007b5214-9cca-4570-b6b1-da66d7e84b70
📒 Files selected for processing (3)
src/frontend/components/_features/[workspace]/editor/device/ethercat/components/__tests__/esi-upload.test.tsxsrc/frontend/components/_features/[workspace]/editor/device/ethercat/components/esi-upload.tsxsrc/middleware/shared/ports/esi-port.ts
| @@ -0,0 +1,96 @@ | |||
| import type { ESIRepositoryItemLight } from '@root/middleware/shared/ports/esi-types' | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if openplc-web#494 is merged and verify file existence
# Check if we can query the web repo (requires GitHub CLI with appropriate access)
echo "=== Checking openplc-web PR `#494` status ==="
gh pr view 494 --repo Autonomy-Logic/openplc-web --json state,title,merged,mergedAt 2>/dev/null || echo "Cannot access openplc-web repo or PR does not exist"
echo -e "\n=== Checking if test file exists in openplc-web ==="
gh api repos/Autonomy-Logic/openplc-web/contents/src/frontend/components/_features/workspace/editor/device/ethercat/components/__tests__/esi-upload.test.tsx 2>/dev/null && echo "File exists in web repo" || echo "File does NOT exist in web repo"Repository: Autonomy-Logic/openplc-editor
Length of output: 381
🏁 Script executed:
# Find CI configuration files
find . -type f \( -name "*.yml" -o -name "*.yaml" \) | grep -E "(workflow|action|ci)" | head -20Repository: Autonomy-Logic/openplc-editor
Length of output: 551
🏁 Script executed:
# Check for documentation about repository sync or relationships
fd -t f -E node_modules -E dist \( -name "README*" -o -name "CONTRIBUTING*" -o -name ".github" \)Repository: Autonomy-Logic/openplc-editor
Length of output: 304
🏁 Script executed:
# Look for any sync-related scripts or configurations
rg "surface.*sync|sync.*surface" --type ts --type tsx --type js --type jsx --type yml --type yaml -l 2>/dev/null | head -20Repository: Autonomy-Logic/openplc-editor
Length of output: 55
🏁 Script executed:
cat -n ./.github/workflows/ci-sync.ymlRepository: Autonomy-Logic/openplc-editor
Length of output: 19516
🏁 Script executed:
# Check if there's documentation about the sync strategy
rg -i "surface.*sync|byte.*identical|mirror" --type md --type txt -A 3 -B 1Repository: Autonomy-Logic/openplc-editor
Length of output: 1819
🏁 Script executed:
# Look for any environment variables or configuration related to sync
rg "SYNC|surface" .github/workflows/ci-sync.ymlRepository: Autonomy-Logic/openplc-editor
Length of output: 989
🏁 Script executed:
cat -n scripts/compare-surfaces.py | head -100Repository: Autonomy-Logic/openplc-editor
Length of output: 3414
🏁 Script executed:
# Find the SURFACES definition that's referenced in ci-sync.yml
find . -type f -name "*.py" -o -name "*.json" -o -name "*.ts" -o -name "*.tsx" | xargs grep -l "SURFACES" 2>/dev/null | grep -v node_modules | head -10Repository: Autonomy-Logic/openplc-editor
Length of output: 144
🏁 Script executed:
# Look for configuration about what constitutes a "surface"
fd -t f "surface" --type f | grep -v node_modules | head -20Repository: Autonomy-Logic/openplc-editor
Length of output: 140
🏁 Script executed:
cat -n scripts/compare-surfaces.py | tail -50Repository: Autonomy-Logic/openplc-editor
Length of output: 2028
🏁 Script executed:
# Check if there are any filters or exclusions for test files in the comparison
grep -n "test\|spec\|__tests__\|skip\|exclude\|ignore" scripts/compare-surfaces.pyRepository: Autonomy-Logic/openplc-editor
Length of output: 55
🏁 Script executed:
# Verify the exact test file path structure in the editor repo
find src/frontend -path "*esi-upload.test.tsx" -o -path "*esi-upload*" 2>/dev/nullRepository: Autonomy-Logic/openplc-editor
Length of output: 278
Test file must be synced to openplc-web repository.
Test files are included in the byte-identical surface comparison (no exclusions exist in compare-surfaces.py). This file exists in the editor repo but is missing from the web repo, which will cause the CI surface-sync check to fail.
The sync check will report an error and block the merge unless:
- A corresponding web PR (such as
#494) with this file is merged first, OR - The file is added to the web repo on the same target branch.
Coordinate with the web repository to ensure this test file is synced before merging.
🧰 Tools
🪛 GitHub Actions: CI / 3_sync _ Shared Surface Sync.txt
[error] 1-1: compare-surfaces.py reported a mismatch: file exists only in editor repo (not in web/src).
🪛 GitHub Actions: CI / sync _ Shared Surface Sync
[error] 1-1: [frontend] File exists only in editor repo
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/frontend/components/_features/`[workspace]/editor/device/ethercat/components/__tests__/esi-upload.test.tsx
at line 1, The test file
src/frontend/components/_features/[workspace]/editor/device/ethercat/components/__tests__/esi-upload.test.tsx
is present in this repo but missing from the openplc-web repository, causing the
surface-sync CI check to fail; either add the identical file to the web repo on
the same target branch or coordinate with the web repo maintainers to merge the
corresponding PR (e.g., the one that should contain esi-upload.test.tsx) before
merging this PR so the byte-identical test is present in both repos and the
compare-surfaces.py check passes.
| // A recovered-but-unlisted upload (dedupAfterRetry) is on the backend yet | ||
| // missing from `newItems`. Re-list the repository so it shows up; fall | ||
| // back to the locally accumulated list if the refresh itself fails. | ||
| if (needsRepositoryRefresh) { | ||
| const refreshed = await esi!.loadRepositoryLight() | ||
| if (refreshed.success && refreshed.items) { | ||
| onFilesLoaded(refreshed.items, errors.length > 0 ? errors : undefined) | ||
| return | ||
| } | ||
| } | ||
|
|
||
| onFilesLoaded([...repository, ...newItems], errors.length > 0 ? errors : undefined) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check IPC bridge type for esiParseAndSaveFile
# Search for the bridge method signature
rg -n -A5 'esiParseAndSaveFile.*Promise' --type=tsRepository: Autonomy-Logic/openplc-editor
Length of output: 55
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Find all occurrences of dedupAfterRetry
rg -n --hidden --no-ignore-vcs "dedupAfterRetry" . || true
# 2) Find ESI port / loadRepositoryLight in the repo
rg -n --hidden --no-ignore-vcs "loadRepositoryLight" src || true
rg -n --hidden --no-ignore-vcs "EsiPort" src || true
# 3) Inspect main/renderer IPC bridge files for any ESI-related methods
rg -n --hidden --no-ignore-vcs "(esi|Esi).*Parse|esiParse|loadRepository" src/main/modules/ipc || true
rg -n --hidden --no-ignore-vcs "window\\.bridge\\." src/middleware/adapters/editor || true
# 4) Directly search for IPC method declarations mentioning ESI in renderer bridge
rg -n --hidden --no-ignore-vcs "ipc.*esi|esi.*ipc|loadRepositoryLight|RepositoryLight" src/main/modules/ipc/renderer.ts || trueRepository: Autonomy-Logic/openplc-editor
Length of output: 50385
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# If dedupAfterRetry exists, print surrounding type declarations in main IPC renderer bridge
# (Use a small window to avoid huge output)
for pat in "dedupAfterRetry" "loadRepositoryLight" "RepositoryLight" "needsRepositoryRefresh" ; do
echo "---- pattern: $pat ----"
rg -n -A12 -B4 "$pat" src/main/modules/ipc/renderer.ts src/main/modules/ipc/main.ts || true
doneRepository: Autonomy-Logic/openplc-editor
Length of output: 6062
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Print EsiPort contract for parseAndSaveFile (includes dedupAfterRetry)
sed -n '1,120p' src/middleware/shared/ports/esi-port.ts
# Print exact IPC renderer bridge signature for esiParseAndSaveFile
# (small window around the line indicated in the earlier grep)
sed -n '540,640p' src/main/modules/ipc/renderer.ts
# Print editor ESI adapter mapping for parseAndSaveFile
sed -n '1,140p' src/middleware/adapters/editor/esi-adapter.tsRepository: Autonomy-Logic/openplc-editor
Length of output: 11447
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' src/frontend/components/_features/[workspace]/editor/device/ethercat/components/esi-upload.tsxRepository: Autonomy-Logic/openplc-editor
Length of output: 7707
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the main-process handler implementation
rg -n "handleESIParseAndSaveFile" src/main/modules/ipc/main.ts
# Print the handler body
rg -n -A60 "handleESIParseAndSaveFile" src/main/modules/ipc/main.ts
# Locate backend ESI service parse method
rg -n "parseAndSaveFile" src/backend -S
# Print likely method signature / return type around dedupAfterRetry
rg -n -A40 -B5 "dedupAfterRetry" src/backend/editor src/backend/shared src/backend -S || trueRepository: Autonomy-Logic/openplc-editor
Length of output: 6076
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Print the parseAndSaveFile implementation area
rg -n -A140 -B20 "async parseAndSaveFile" src/backend/editor/ethercat/esi-service.ts
# Also look for dedupAfterRetry usage in backend
rg -n "dedupAfterRetry" src/backend/editor/ethercat/esi-service.ts src/backend/shared -S || trueRepository: Autonomy-Logic/openplc-editor
Length of output: 5383
Propagate dedupAfterRetry consistently through backend → IPC bridge → EsiPort adapter (or remove the UI dead branch)
EsiPort.parseAndSaveFilealready modelsdedupAfterRetry?: booleaninsrc/middleware/shared/ports/esi-port.ts.src/backend/editor/ethercat/esi-service.tsparseAndSaveFilecurrently never returnsdedupAfterRetry(onlyitem?,duplicate?,error?).src/main/modules/ipc/renderer.tsesiParseAndSaveFilereturn type also omitsdedupAfterRetry.src/middleware/adapters/editor/esi-adapter.tsdrops any such field (it maps onlyitemandduplicate).
This makes the ESIUpload path that sets needsRepositoryRefresh on result.dedupAfterRetry effectively unreachable in the real flow; it only works when mocked in tests. Fix by either implementing dedupAfterRetry end-to-end (service → IPC bridge → adapter) or removing dedupAfterRetry from the port/UI until the backend supports it.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/frontend/components/_features/`[workspace]/editor/device/ethercat/components/esi-upload.tsx
around lines 116 - 127, The UI branch relying on result.dedupAfterRetry is dead
because dedupAfterRetry isn't propagated end-to-end; either implement
propagation or remove the UI branch. To implement propagation: update backend
function parseAndSaveFile in esi-service.ts to set and return
dedupAfterRetry:boolean when appropriate, update the IPC handler
esiParseAndSaveFile in src/main/modules/ipc/renderer.ts to include
dedupAfterRetry in its response, and update the middleware adapter
editor/esi-adapter.ts mapping to pass dedupAfterRetry through when calling
EsiPort.parseAndSaveFile. Alternatively, remove the
needsRepositoryRefresh/dedupAfterRetry handling from the UI component
(esi-upload.tsx) to match current backend behavior.
JoaoGSP
left a comment
JoaoGSP
left a comment
There was a problem hiding this comment.
Mirror review of Autonomy-Logic/openplc-web#494 — the three shared files are byte-identical across both PRs, so the shared-surface findings apply to both. The new test suite was additionally verified to pass under this repo's Jest setup.
| @@ -84,8 +90,13 @@ const ESIUpload = ({ onFilesLoaded, repository, isLoading = false }: ESIUploadPr | |||
|
|
|||
| if (result.success && result.item) { | |||
| newItems.push(result.item) | |||
There was a problem hiding this comment.
🔴 Duplicate-row bug in the recovery path (reachable via the web adapter; dead code here today, but this is the shared surface). A recovered item can already be present in the repository prop: if the file's content existed in the repo before the upload and attempt 1 fails transiently before reaching the backend, attempt 2 hits the SHA-256 dedup against the pre-existing row. The web adapter recovers that row by filename and returns { success, item, dedupAfterRetry }, this branch pushes it into newItems, and onFilesLoaded([...repository, ...newItems]) then contains the same row twice — duplicated table row and duplicate React key (key={item.id} in esi-repository-table.tsx). Suggest deduping the merged list by id before calling onFilesLoaded.
| @@ -39,11 +39,17 @@ export interface EsiPort { | |||
| * Dedup is filename-based: reimporting the same bytes under a different name | |||
There was a problem hiding this comment.
🟡 Port doc now contradicts the backend. This pre-existing paragraph says "Dedup is filename-based: reimporting the same bytes under a different name will add a new entry", but the web backend dedups by SHA-256 content hash (autonomy-edge upload-esi-file.use-case.ts: "dedup by SHA-256"), and the new adapter comments added in the mirror PR rely on exactly that. Since this PR extends this doc block, the stale filename-based paragraph should be updated so the port contract, adapter comments, and backend all agree.
| * will add a new entry, and replacing a file's contents without renaming it | ||
| * is reported as a duplicate. | ||
| * | ||
| * `dedupAfterRetry: true` indicates the duplicate response landed after a |
There was a problem hiding this comment.
🟡 dedupAfterRetry contract under-documented. Two gaps: (a) the doc doesn't say which adapters can produce the flag — this repo's local IPC adapter never sets it, so the ESIUpload branch handling it is intentionally dead code in openplc-editor, and this doc is the only place an editor-side reader can learn that; (b) it describes only one of the three result shapes callers can actually receive — the web adapter also returns dedupAfterRetry: true together with item (recovered add, handled by the result.item branch) and alongside duplicate: true (failed recovery). Suggest spelling out the matrix: item + dedupAfterRetry = recovered add; duplicate + dedupAfterRetry = persisted but unlisted, refresh recommended; duplicate alone = real dedup, silent skip.
| import { fireEvent, render, waitFor } from '@testing-library/react' | ||
|
|
||
| // Mocked EsiPort surface — the two methods the upload flow touches. The | ||
| // `mock`-prefixed name is referenced lazily inside the factory (only when |
There was a problem hiding this comment.
🟡 Comment credits the wrong mechanism for Jest compatibility. ts-jest's hoist transformer only hoists jest.mock, not vi.mock — under this repo's Jest the test passes because import { ESIUpload } is deliberately placed after the vi.mock call, not because of the lazy mock-prefixed reference (that only matters under Vitest, where the factory is hoisted). Suggest the comment state both mechanisms and mark the import position as load-bearing, so nobody "fixes" it by moving the import into the top import block. (Verified: the suite passes under this repo's Jest as-is.)
2 participants
What
Shared-surface sync with openplc-web PR Autonomy-Logic/openplc-web#494.
Mirrors, byte-identical, the three shared-surface files from that PR:
middleware/shared/ports/esi-port.ts— thededupAfterRetrycontract field onparseAndSaveFile.frontend/.../ethercat/components/esi-upload.tsx— theESIUploadconsumer that re-lists the repository when an upload is recovered after a transient-failure retry but comes back without an item, so the file surfaces instead of silently vanishing.frontend/.../components/__tests__/esi-upload.test.tsx— the four-branch test (normal add, dedup-after-retry refetch, refetch-failure fallback, real duplicate). Runner-agnostic mock, runs under Jest here and Vitest on web.Why
Keeps the shared surface in lockstep with openplc-web (enforced by the cross-repo surface-sync CI). The retry/adapter internals are web-only and intentionally not ported — the editor's main-process ESI adapter doesn't retry, and the new
dedupAfterRetryfield is optional, so this is backward compatible.Tests
esi-upload.test.tsxpasses under the editor's Jest (4/4).🤖 Generated with Claude Code
Summary by CodeRabbit